Move arm setup scripts into .ci/docker and create links to them #1574
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/1574
Note: Links to docs will display an error until the docs builds have been completed. ✅ You can merge normally! (5 Unrelated Failures)As of commit 559cd99 with merge base 52a4cec ( FLAKY - The following job failed but was likely due to flakiness present on trunk:
BROKEN TRUNK - The following jobs failed but was present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
facebook-github-bot
added
the
CLA Signed
huydhn
changed the title
|
@huydhn has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@huydhn has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| @@ -0,0 +1,246 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
I am not sure if we want to move examples dir under docker. This was meant as an example for the end user, it happens to be a good setup for CI.
There was a problem hiding this comment.
I kind of agree on this point, it looks weird having a bunch of scripts under .ci/docker/examples/arm, so I try to think of them as installation scripts to setup arm job and see how it goes.
There are some rules w.r.t. the building of Docker images:
- Only
.ci/dockerfolder is taken into account per convention. This PR tries to follow this rule, but it looks bad in this case. - The rebuilding of the Docker image happens. But because its hash version, calculated by
DOCKER_TAG=$(git rev-parse HEAD:"${DOCKER_BUILD_DIR}"), doesn't change, the image becomes stale.
I think I'll keep this PR around just in case, and see if I can bend the second rule instead to force update the image. Note that force update, even if it could be done, can only be an optional choice because not overwriting an existing image helps CI reliability from my experience.
IIRC, we used to have this option, but nothing was using it. I have found an use case for this in pytorch/executorch#1335 (comment) where ExecuTorch team copies the content of `examples/arm` into `.ci/docker` before starting the build https://github.com/pytorch/executorch/blob/main/.ci/docker/build.sh#L40-L46: * The docker image was rebuilt, but its hash tag remained the same because the copy step above is not version tracked. * The new image was not pushed. I have tried to move `examples/arm` into `.ci/docker` and setup softlinks, but the result looks weird pytorch/executorch#1574 (comment), so I'll probably drop that approach. The remaining option here is to add a `force-push` flag that can be used to, well, force push the image even if its hash tag already exists.
Summary: Instead of #1574, the other option to resolve the stale Docker image issue from #1335 (comment) is to use the new `force-push` flag added by pytorch/test-infra#4866. This ensures that a rebuilt image will always be pushed upstream even if its hash tag already exists. ### Testing With the new GH action, the image is pushed correctly https://github.com/pytorch/executorch/actions/runs/7483947498/job/20369991072?pr=1582#step:6:49039 Pull Request resolved: #1582 Reviewed By: digantdesai Differential Revision: D52684112 Pulled By: huydhn fbshipit-source-id: a7642c47be3681129802a4405e5cdcdc2b328560
3 participants
All files used to build the Docker image needs to be in
.ci/dockerolder so that the image hash is updated correctly when they change. The good news is that links can be setup to refer to them from other locations. This complication is partly due to Docker build rule to only access files under its build folder, a.k.a.ci/dockerI move some file around in this PR as follows:
This workaround allows people to edit files in
examples/armas usual because they link to the source files under.ci/dockerwhile updating the Docker image hash correctly.